HDDS-8383. Misreplication cannot be resolved with single rack #4539

adoroszlai · 2023-04-05T11:19:03Z

What changes were proposed in this pull request?

The default topology contains a single rack. With rack-aware container placement policy (HDDS-8300), overreplication is considered a misreplication, since more replicas are in a single rack than desired. Yet misreplication cannot be resolved since there is no other rack.

This can be reproduced by configuring rack-awareness for integration tests:

diff --git hadoop-ozone/integration-test/src/test/resources/ozone-site.xml hadoop-ozone/integration-test/src/test/resources/ozone-site.xml
index 0c5ae1fa88..87a85dc1f1 100644
--- hadoop-ozone/integration-test/src/test/resources/ozone-site.xml
+++ hadoop-ozone/integration-test/src/test/resources/ozone-site.xml
@@ -31,4 +31,9 @@
     <value>4</value>
   </property>
 
+  <property>
+    <name>ozone.scm.container.placement.impl</name>
+    <value>org.apache.hadoop.hdds.scm.container.placement.algorithms.SCMContainerPlacementRackAware</value>
+  </property>
+
 </configuration>

and running:

$ mvn -am -pl :ozone-integration-test -Dtest='TestDecommissionAndMaintenance#testContainerIsReplicatedWhenAllNodesGotoMaintenance' clean test
...
	at org.apache.hadoop.ozone.scm.node.TestDecommissionAndMaintenance.waitForContainerReplicas(TestDecommissionAndMaintenance.java:757)
	at org.apache.hadoop.ozone.scm.node.TestDecommissionAndMaintenance.testContainerIsReplicatedWhenAllNodesGotoMaintenance(TestDecommissionAndMaintenance.java:431)
...
[ERROR] Errors: 
[ERROR]   TestDecommissionAndMaintenance.testContainerIsReplicatedWhenAllNodesGotoMaintenance:431->waitForContainerReplicas:757 » Timeout

Container placement should be considered valid (not misreplicated) if there is only a single rack. This will let the overreplication logic take care of the extra replicas instead of the misreplication one.

https://issues.apache.org/jira/browse/HDDS-8383

How was this patch tested?

The same integration test passed. Also ran unit tests related to topology and placement policy.

https://github.com/adoroszlai/hadoop-ozone/actions/runs/4617349558

siddhantsangwan · 2023-04-06T09:44:26Z

Going through this code in SCMCommonPlacementPolicy:

  public ContainerPlacementStatus validateContainerPlacement(
      List<DatanodeDetails> dns, int replicas) {
    NetworkTopology topology = nodeManager.getClusterNetworkTopologyMap();
    // We have a network topology so calculate if it is satisfied or not.
    int requiredRacks = getRequiredRackCount(replicas);
    int numRacks = topology != null ? topology.getRackCount() : 1;
    if (numRacks == 1 || replicas == 1 || requiredRacks == 1) {
      if (dns.size() > 0) {
        // placement is always satisfied if there is at least one DN.
        return validPlacement;
      } else {
        return invalidPlacement;
      }
    }

I'm wondering if getRequiredRackCount(replicas) should return 1 instead of 2 in this case? Currently in SCMContainerPlacementRackAware the constant value REQUIRED_RACKS, which is 2, is returned.

adoroszlai · 2023-04-10T07:35:57Z

Thanks @siddhantsangwan for taking a look.

I'm wondering if getRequiredRackCount(replicas) should return 1 instead of 2 in this case?

Good point, it might be a more complete fix, and SCMContainerPlacementRackScatter already does that. Also, there are a few places using min(numRacks, requiredRacks), so we might as well get rid of those.

…ad of SCMCommonPlacementPolicy#validateContainerPlacement

kerneltime · 2023-04-10T16:09:00Z

@swamirishi @ashishkumar50 can you please take a look?

siddhantsangwan

Looks good other than a minor comment.

siddhantsangwan · 2023-04-11T06:08:03Z

...ache/hadoop/hdds/scm/container/placement/algorithms/TestContainerPlacementStatusDefault.java

-    stat = new ContainerPlacementStatusDefault(1, 4, 3, 1, Arrays.asList(1, 2));
+    stat = new ContainerPlacementStatusDefault(1, 4, 1, Arrays.asList(1, 2));
    assertFalse(stat.isPolicySatisfied());
-    assertEquals(2, stat.misReplicationCount());


The original code here doesn't make much sense to me because it's saying currentRacks is 1 but the last argument says 1 replica is on 1 rack and 2 replicas on another rack.

swamirishi

This PR changes a bunch of interfaces changing their definition of the class, would be better to discuss this.

swamirishi · 2023-04-11T06:35:30Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/net/NetworkTopology.java

  List<? extends Node> sortByDistanceCost(Node reader,
      List<? extends Node> nodes, int activeLen);
+
+  default int getRackCount() {


Should we have the concept of Racks in Network Topology? Should this particular function go in PlacementPolicy class instead.

@sodonnel I remember us discussing this before.

swamirishi · 2023-04-11T06:40:16Z

.../apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackScatter.java

    }
-    int maxLevel = networkTopology.getMaxLevel();
-    int numRacks = networkTopology.getNumOfNodes(maxLevel - 1);
+    int numRacks = networkTopology.getRackCount();


I guess we can move this particular logic into SCM common placement policy instead of relying on NetworkTopology since NetworkTopology class is meant to be more generic & need not understand racks.

swamirishi · 2023-04-11T06:43:55Z

...rg/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackAware.java

  protected int getRequiredRackCount(int numReplicas) {
-    return REQUIRED_RACKS;
+    int racks = networkTopology != null ? networkTopology.getRackCount() : 1;
+    return Math.min(REQUIRED_RACKS, racks);


From my understanding, this is the only change which fixes the particular issue. I think it would be better to create another refactoring jira, if you want to change the other interfaces.

This reverts commit 7a0b56a.

This reverts commit 3e3adb2.

…nt instead of SCMCommonPlacementPolicy#validateContainerPlacement" This reverts commit 1ef8ae9.

This reverts commit 251eeb7.

This reverts commit 8e6b956.

sodonnel

LGTM

adoroszlai · 2023-04-11T16:18:36Z

@swamirishi would you like to take another look?

swamirishi

LGTM

adoroszlai · 2023-04-11T16:39:03Z

Thanks @siddhantsangwan, @sodonnel, @swamirishi for the review.

adoroszlai added 2 commits April 5, 2023 12:01

HDDS-8383. Misreplication cannot be resolved with single rack

8e6b956

Use new getRackCount method to reduce duplication

251eeb7

adoroszlai self-assigned this Apr 5, 2023

adoroszlai added the scm label Apr 5, 2023

adoroszlai mentioned this pull request Apr 5, 2023

HDDS-8300. Enable topology aware replication and reads by default. #4489

Merged

adoroszlai added bug Something isn't working scm and removed scm labels Apr 5, 2023

adoroszlai requested review from aswinshakil and siddhantsangwan April 5, 2023 12:23

adoroszlai requested a review from sodonnel April 10, 2023 07:21

adoroszlai added 3 commits April 10, 2023 09:47

Fix at root SCMContainerPlacementRackAware#getRequiredRackCount inste…

1ef8ae9

…ad of SCMCommonPlacementPolicy#validateContainerPlacement

Fix TestContainerPlacementStatusDefault

3e3adb2

Apply same fix in PipelinePlacementPolicy

7a0b56a

siddhantsangwan reviewed Apr 11, 2023

View reviewed changes

swamirishi requested changes Apr 11, 2023

View reviewed changes

adoroszlai added 5 commits April 11, 2023 08:51

Revert "Apply same fix in PipelinePlacementPolicy"

2d59517

This reverts commit 7a0b56a.

Revert "Fix TestContainerPlacementStatusDefault"

7e871b3

This reverts commit 3e3adb2.

Revert "Fix at root SCMContainerPlacementRackAware#getRequiredRackCou…

bfd3791

…nt instead of SCMCommonPlacementPolicy#validateContainerPlacement" This reverts commit 1ef8ae9.

Revert "Use new getRackCount method to reduce duplication"

a261e76

This reverts commit 251eeb7.

Revert "HDDS-8383. Misreplication cannot be resolved with single rack"

8731b61

This reverts commit 8e6b956.

adoroszlai marked this pull request as draft April 11, 2023 06:54

adoroszlai added 2 commits April 11, 2023 08:54

HDDS-8383. Misreplication cannot be resolved with single rack

8ca9e43

Merge remote-tracking branch 'origin/master' into HDDS-8383

3e1a0e7

adoroszlai marked this pull request as ready for review April 11, 2023 07:44

sodonnel approved these changes Apr 11, 2023

View reviewed changes

siddhantsangwan approved these changes Apr 11, 2023

View reviewed changes

swamirishi approved these changes Apr 11, 2023

View reviewed changes

adoroszlai merged commit 45e8c5c into apache:master Apr 11, 2023

adoroszlai deleted the HDDS-8383 branch April 11, 2023 16:38

HDDS-8383. Misreplication cannot be resolved with single rack #4539

HDDS-8383. Misreplication cannot be resolved with single rack #4539

Uh oh!

Conversation

adoroszlai commented Apr 5, 2023

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

siddhantsangwan commented Apr 6, 2023

Uh oh!

adoroszlai commented Apr 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kerneltime commented Apr 10, 2023

Uh oh!

siddhantsangwan left a comment

Choose a reason for hiding this comment

Uh oh!

siddhantsangwan Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

swamirishi left a comment

Choose a reason for hiding this comment

Uh oh!

swamirishi Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

swamirishi Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

swamirishi Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

swamirishi Apr 11, 2023

Choose a reason for hiding this comment

Uh oh!

sodonnel left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented Apr 11, 2023

Uh oh!

swamirishi left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented Apr 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adoroszlai commented Apr 10, 2023 •

edited

Loading